Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/angular cli config #7485

Closed
wants to merge 4 commits into from
Closed

Fix/angular cli config #7485

wants to merge 4 commits into from

Conversation

JounQin
Copy link
Contributor

@JounQin JounQin commented Jul 19, 2019

Issue:

What I did

outputPath, scripts, styles are required by @angular-devkit/build-angular, make sure it won't break if they are not presented.

How to test

  • Is this testable with Jest or Chromatic screenshots?
    Yes, added.

  • Does this need a new example in the kitchen sink apps?
    No need.

  • Does this need an update to the documentation?
    No need.

If your answer is yes to any of these, please make sure to include it in your PR.

@vercel
Copy link

vercel bot commented Jul 19, 2019

This pull request is automatically deployed with Now.
To access deployments, click Details below or on the icon next to each push.

Latest deployment for this branch: https://monorepo-git-fork-jounqin-fix-angularcliconfig.storybook.now.sh

Copy link
Member

@kroeder kroeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! LGTM
Don't know why a couple of CI tests don't pass - I'll look into this 🙂

@stale stale bot removed the inactive label Aug 30, 2019
@ndelangen ndelangen self-assigned this Aug 30, 2019
@ndelangen
Copy link
Member

ndelangen commented Aug 30, 2019

Something in the test makes to unable to finish

@JounQin could you have a look what it might be?

@JounQin
Copy link
Contributor Author

JounQin commented Aug 31, 2019

@ndelangen Sorry but It should not be my fault, all the merged commits have error status. I've rebased from next branch, hope it will work.

@stale stale bot added the inactive label Sep 21, 2019
@ndelangen
Copy link
Member

@kroeder I merged in next, hope the CI approves this time. Would you be able to give your final review?

@kroeder
Copy link
Member

kroeder commented Sep 28, 2019

ci tests still have errors :| I also have trouble getting them green in #7747
The changes in this PR LGTM

@kroeder
Copy link
Member

kroeder commented Sep 28, 2019

I changed the cli test in e187e79

I also split functions in order to improve the tests. If I get CI green we can merge it in this branch. I hope this helps

@stale
Copy link

stale bot commented Oct 24, 2019

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Oct 24, 2019
@JounQin
Copy link
Contributor Author

JounQin commented Oct 24, 2019

So the CI error has not been fixed?

And as I can see, the PR should not be prevented from merging like other merged PRs.

@stale stale bot removed the inactive label Oct 24, 2019
@shilman
Copy link
Member

shilman commented Oct 24, 2019

 FAIL  app/angular/src/server/__tests__/angular-cli_utils.test.ts
  ● Test suite failed to run

    TypeError: Cannot read property 'filter' of undefined

      1 | import fs from 'fs';
      2 | import { basename, dirname, normalize, relative, resolve, Path } from '@angular-devkit/core';
    > 3 | import {
        | ^
      4 |   getCommonConfig,
      5 |   getStylesConfig,
      6 | } from '@angular-devkit/build-angular/src/angular-cli-files/models/webpack-configs';

      at loadTsLibFiles (node_modules/@angular-devkit/build-angular/node_modules/@ngtools/webpack/src/transformers/ast_helpers.js:109:41)
      at Object.<anonymous> (node_modules/@angular-devkit/build-angular/node_modules/@ngtools/webpack/src/transformers/ast_helpers.js:46:20)
      at Object.<anonymous> (node_modules/@angular-devkit/build-angular/node_modules/@ngtools/webpack/src/transformers/index.js:14:10)
      at Object.<anonymous> (node_modules/@angular-devkit/build-angular/node_modules/@ngtools/webpack/src/angular_compiler_plugin.js:26:24)
      at Object.<anonymous> (node_modules/@angular-devkit/build-angular/node_modules/@ngtools/webpack/src/index.js:13:10)
      at Object.<anonymous> (node_modules/@angular-devkit/build-angular/src/angular-cli-files/models/webpack-configs/typescript.js:14:19)
      at Object.<anonymous> (node_modules/@angular-devkit/build-angular/src/angular-cli-files/models/webpack-configs/index.js:18:10)
      at Object.<anonymous> (app/angular/src/server/angular-cli_utils.ts:3:1)
      at Object.<anonymous> (app/angular/src/server/__tests__/angular-cli_utils.test.ts:3:1)

@shilman
Copy link
Member

shilman commented Oct 24, 2019

@JounQin @kroeder The test that's failing is the test that's introduced in this PR. We can't merge it in the current state, otherwise our builds would be broken for all PRs.

@JounQin
Copy link
Contributor Author

JounQin commented Oct 24, 2019

@shilman Are you sure? The error message you mentioned seems having nothing related to this PR.

@shilman
Copy link
Member

shilman commented Oct 24, 2019

@JounQin When I click on the "Files changed" tab in this PR, it looks like that file was added in this PR. No?

@ndelangen
Copy link
Member

You added the test here:
df76d44

👀

@stale
Copy link

stale bot commented Nov 14, 2019

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Nov 14, 2019
@ndelangen ndelangen closed this Nov 16, 2019
@JounQin JounQin deleted the fix/angular_cli_config branch July 29, 2020 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants